Skip to content

Conversation

@kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Dec 5, 2025

Note

Implements FDv2 polling (requestor + data source) with event-array parsing, ETag/304 handling, status updates, env-id header extraction, and comprehensive tests.

  • FDv2 Polling:
    • Data Source: New Internal/FDv2DataSources/FDv2PollingDataSource.cs implementing periodic polling, processing protocol actions, applying transactional changesets, marking initialization, updating DataSourceStatus, and handling JSON/HTTP errors.
    • Requestor: New Internal/FDv2DataSources/FDv2PollingRequestor.cs building requests with selector query params, ETag caching, 304 handling, header capture (including x-ld-envid), and deserializing polling events.
    • Interface/Models: New IFDv2PollingRequestor.cs defining FDv2PollingResponse and requestor interface.
    • Event Parsing: Internal/FDv2Payloads/FDv2Event.cs adds DeserializeEventsArray and poll-event converter usage for parsing events arrays with strict validation.
  • Streaming:
    • Minor imports added in FDv2StreamingDataSource.cs (no functional change in this diff).
  • Misc:
    • Minor cleanup in Internal/DataSources/FeatureRequestor.cs.
  • Tests:
    • Extensive tests in test/Internal/FDv2DataSources/FDv2PollingDataSourceTest.cs covering init, full/partial updates, deletes, 304 behavior, error/status transitions, selector propagation, and env-id extraction.
    • Additional tests in test/Internal/FDv2Payloads/FDv2PayloadsTest.cs for DeserializeEventsArray and valid/invalid payload scenarios.

Written by Cursor Bugbot for commit 69b18d1. This will update automatically on new commits. Configure here.

@kinyoklion kinyoklion force-pushed the rlamb/sdk-1584/fdv2-polling-data-source branch 4 times, most recently from 0273b89 to bfa1caa Compare December 5, 2025 21:40
@kinyoklion kinyoklion force-pushed the rlamb/sdk-1584/fdv2-polling-data-source branch 2 times, most recently from aa1998d to 0187fcf Compare December 5, 2025 22:19
@kinyoklion kinyoklion force-pushed the rlamb/sdk-1584/fdv2-polling-data-source branch from 0187fcf to aee3377 Compare December 5, 2025 22:59
@kinyoklion
Copy link
Member Author

bugbot review

_log.Debug("Get all flags returned 304: not modified");
return null;
}
//We ensure the status code after checking for 304, because 304 isn't considered success
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this comment because it was blatantly wrong. A 304 is a success, and this code doesn't even run when there is a 304 because we return above. (I looked back through the history and it has been incorrect since addition 9 years ago.)

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no bugs!


@kinyoklion kinyoklion marked this pull request as ready for review December 5, 2025 23:16
@kinyoklion kinyoklion requested a review from a team as a code owner December 5, 2025 23:16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be FDv2Requestor? The only logic inside it that is specific to polling is the logs. I know off the top of my head iOS and Node have Requestor classes that are separable from the polling itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The payload it produces is not agnostic, so I am not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always change it though, as it is internal.

@kinyoklion kinyoklion merged commit a50515e into main Dec 8, 2025
15 checks passed
@kinyoklion kinyoklion deleted the rlamb/sdk-1584/fdv2-polling-data-source branch December 8, 2025 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants